Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Emit MsgsNoticed on receipt of an IMAP-seen message #6351

Closed
wants to merge 1 commit into from

Conversation

iequidoo
Copy link
Collaborator

@iequidoo iequidoo commented Dec 20, 2024

imap::Session::sync_seen_flags() emits MsgsNoticed for existing messages seen on other devices, so receive_imf should do the same when it receives a seen message. Otherwise a multi-device user may see a new message notification on device A, just swipe it, then see another new message notification and mark it as read, and when their device B goes online, it will show a notification for the first message, and it won't be removed because MsgsNoticed isn't emitted. I have checked this with my DC Android and Desktop. With this fix the notification should be removed at least.

EDIT: The fix doesn't help DC Desktop not show a notification for the first message. I didn't check Android though. Anyway, MsgsNoticed was missing obviously.

`imap::Session::sync_seen_flags()` emits `MsgsNoticed` for existing messages seen on other devices,
so `receive_imf` should do the same when it receives a seen message. Otherwise a multi-device user
may see a new message notification on device A, just swipe it, then see another new message
notification and mark it as read, and when their device B goes online, it will show a notification
for the first message, and it won't be removed because `MsgsNoticed` isn't emitted. I have checked
this with my DC Android and Desktop. With this fix the notification should be removed at least.
@Hocuri
Copy link
Collaborator

Hocuri commented Jan 6, 2025

This PR would mean that when any messages with state InSeen is received (i.e. also an mdn or reaction), MsgsNoticed will be emitted and the notifications for this chat will be removed. Since a missing notification is way worse than one too many (and a missing notification can lead to social issues, like missing some important info or not answering - esp if someone doesn't use DC a lot), we shouldn't do this. While this would be fixable, this would again both introduce some risk that we emit MsgsNoticed even though we shouldn't and increase complexity.

Since all of this is a very small bug, I suggest to simply not fix it at all. One could even argue that this is the correct behavior (after all, the first notification was only swiped away, not marked as read, so the message should be notified on the other device).

@Hocuri Hocuri closed this Jan 6, 2025
@iequidoo
Copy link
Collaborator Author

iequidoo commented Jan 6, 2025

This PR would mean that when any messages with state InSeen is received (i.e. also an mdn or reaction), MsgsNoticed will be emitted

Received MDNs and reactions aren't messages currently, chat_id for them is DC_CHAT_ID_TRASH (#6213 which makes received reactions to be messages isn't merged). But if we don't want to emit MsgsNoticed when we're not sure all messages are really noticed, we should remove the same code from imap, otherwise currently the code is inconsistent (imap as well may emit MsgsNoticed when not all messages are noticed). I'll make another PR then

@Hocuri
Copy link
Collaborator

Hocuri commented Jan 7, 2025

To me, the code very much looks like the MsgsNoticed event would still have been emitted, since we don't early-return from receive_imf in this case, even if chat_id is trash. Anyway, before merging a PR like this, we would have had to test this on a real device (not just read the code) in order not to risk introducing a severe bug. But as I said, I don't think it's worth it spending energy on this, things are fine already.

@adbenitez
Copy link
Member

adbenitez commented Jan 7, 2025

at least for reactions it is quite annoying to receive the notifications doubled in every device, what is worse, for every reaction change for the same message, several notifications remain, so you see wrong notifications of older reactions that got changed or even retracted and open the chat and see nothing

for example this are all notifications for every reaction change from the same user to the same message:
image

@Hocuri
Copy link
Collaborator

Hocuri commented Jan 7, 2025

#6213 will fix the problem of reactions being shown on multiple devices.

The problem of wrong notifications of older reactions that got changed is probably easier fixed in the UI, at least I can't come up with a sensible way how core could help with that.

FTR, if we have an actual user value for emitting MsgsNoticed on receipt of an IMAP-seen message, and test that MsgsNoticed is not emitted for every reaction and mdn, then I'm not against it.

@iequidoo
Copy link
Collaborator Author

iequidoo commented Jan 7, 2025

To me, the code very much looks like the MsgsNoticed event would still have been emitted, since we don't early-return from receive_imf in this case, even if chat_id is trash.

True, it will be emitted even for MDNs, but with DC_CHAT_ID_TRASH. I think UIs shouldn't remove any notifications in this case. Btw, DC Desktop doesn't remove any notifications for me even if the chat isn't trash and i couldn't understand why. Anyway, i think a check that the chat isn't trash should be added at least to not emit useless events. But now i think we are not going to reopen this PR anyway.

Anyway, before merging a PR like this, we would have had to test this on a real device (not just read the code) in order not to risk introducing a severe bug. But as I said, I don't think it's worth it spending energy on this, things are fine already.

The problem is that the code is inconsistent already now. I can't understand why imap emits MsgsNoticed when some random message was seen on IMAP, but receive_imf doesn't do so if it receives such a message. There are no tests for this, so i think the code is what it is just by accident. Both approaches (always emitting an event and always not) have their downsides, but indeed, not to spend much energy on this, i suggest to just unify the code in the safest way and add a simple test.

The current problem is that sync_seen_flags() is called after fetching new messages, so when the device goes online, MsgsNoticed may remove useful notifications. So the safest way is to not emit the event in both places. For "usual" messages this shouldn't cause problems, notifications will be removed for seen message from all devices anyway. But for "swiped out" messages notifications will remain and i think that's rather good. This way i can swipe out the message because i can't reply to it from the current device and do that later from another one (this really happens for me, sometimes i need a convenient keyboard etc.). For reactions the problem will be resolved by #6213 anyway. So i think #6413 is the right way, it's a one-line change and i'm going to add a simple Python test for it.

EDIT: I re-read the code again and now i've understood why we emit MsgsNoticed in sync_seen_flags() -- otherwise there are no other events that remove notifications for messages seen on other devices. So while we may also remove useful notifications (because the current device may have newer messages when it goes online), we have no other choice. Still we can fix this a little if we call sync_seen_flags() before receiving new messages -- this way notifications for new messages remain.

Then anyway we should reopen the PR because in receive_imf emitting MsgsNoticed is safer because that doesn't remove notifications for newer messages and this way we will unify the code and behaviour.

@iequidoo
Copy link
Collaborator Author

iequidoo commented Jan 8, 2025

Recreated the PR: #6415. Can't reopen this one because i force-pushed the branch or maybe recreated it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants